Only use 'main' Parts of speech. Like FLEx. #2084

myieye · 2025-10-29T16:35:01Z

This excludes e.g. Reversel index parts of speech.

coderabbitai · 2025-10-29T16:35:21Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

GetPartsOfSpeech() now reads parts of speech from Cache.LangProject.AllPartsOfSpeech (cast to IPartOfSpeech) instead of PartOfSpeechRepository.AllInstances(); test teardown DisposeAsync now deletes PartOfSpeech entries from both FwDataApi and CrdtApi.

Changes

Cohort / File(s)	Change Summary
API data source change `backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`	Switched GetPartsOfSpeech() to source items from `Cache.LangProject.AllPartsOfSpeech` (cast to `IPartOfSpeech`); kept ordering by `Name.BestAnalysisAlternative.Text`, async conversion, and mapping via `FromLcmPartOfSpeech`.
Test teardown cleanup `backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs`	`DisposeAsync` augmented to iterate and delete all PartOfSpeech items from `FwDataApi` and to fetch (`ToArrayAsync`) and delete all PartOfSpeech items from `CrdtApi`.
New unit test `backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/PartOfSpeechTests.cs`	Added `GetPartsOfSpeech_DoesNotReturnReversalIndexPos()` test that creates a reversal-index POS, calls GetPartsOfSpeech(), and asserts the reversal-index POS is excluded.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Verify the cast to IPartOfSpeech is safe for all entries in Cache.LangProject.AllPartsOfSpeech.
Confirm ordering and mapping behavior match previous results from PartOfSpeechRepository.AllInstances().
Check test teardown properly removes items and handles async disposal without leaving dangling test data.
Review the new test for correct setup/teardown and that it accurately asserts exclusion of reversal-index POS.

Suggested reviewers

imnasnainaec

Poem

🐰
I hop through caches soft and deep,
I fetch the words that silence keeps.
I tidy tests, remove the traces,
And leave no crumbs in hidden places. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description check	❓ Inconclusive	The pull request description is related to the changeset but is very vague and lacks meaningful detail about the implementation.	Provide more specific details about what changes were made, which files were modified, and how the solution ensures only 'main' parts of speech are returned.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Only use 'main' Parts of speech. Like FLEx.' clearly and specifically describes the main change: filtering parts of speech to exclude non-main types like reversal index POS, aligning behavior with FLEx.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch only-use-main-parts-of-speech

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-10-29T16:39:46Z

UI unit Tests

1 files ±0 45 suites ±0 19s ⏱️ ±0s
111 tests ±0 111 ✅ ±0 0 💤 ±0 0 ❌ ±0
160 runs ±0 160 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit c552964. ± Comparison against base commit 846d917.

♻️ This comment has been updated with latest results.

argos-ci · 2025-10-29T16:40:28Z

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build	Status	Details	Updated (UTC)
default (Inspect)	✅ No changes detected	-	Nov 11, 2025, 11:23 AM

myieye · 2025-11-03T14:04:06Z

Note, for some reason this change found what I consider to be a bug in our fwdata api (or maybe even LibLcm?).
Without deleting parts of speech after each test run, a duplicate pos started being returned from FwData's GetPartsOfSpeech.
#1931 (comment)

myieye · 2025-11-03T15:09:38Z

@coderabbitai full review

coderabbitai · 2025-11-03T15:09:51Z

✅ Actions performed

Full review triggered.

This excludes e.g. Reversel index parts of speech.

… Reversel index parts of speech.

myieye · 2025-11-11T11:42:42Z

@coderabbitai full review

coderabbitai · 2025-11-11T11:43:24Z

✅ Actions performed

Full review triggered.

Copilot

Pull Request Overview

This PR modifies the GetPartsOfSpeech() method to only return 'main' parts of speech (like FLEx does), excluding reversal index parts of speech. This change ensures consistency with how FLEx handles parts of speech.

Key changes:

Changed the data source from PartOfSpeechRepository.AllInstances() to Cache.LangProject.AllPartsOfSpeech to filter out reversal index POS
Added a test to verify that reversal index parts of speech are excluded
Added cleanup of parts of speech in SyncTests.DisposeAsync() to maintain test isolation

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
FwDataMiniLcmApi.cs	Modified `GetPartsOfSpeech()` to use `Cache.LangProject.AllPartsOfSpeech` instead of `PartOfSpeechRepository.AllInstances()`
PartOfSpeechTests.cs	Added test to verify reversal index POS are not returned by `GetPartsOfSpeech()`
SyncTests.cs	Added cleanup logic in `DisposeAsync()` to delete parts of speech created during tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs

jasonleenaylor

LGTM, in other news I thought I'd reviewed this ages ago...

github-actions bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Oct 29, 2025

myieye mentioned this pull request Nov 3, 2025

A project can become un-openable if you create the same example twice #1931

Open

myieye marked this pull request as ready for review November 3, 2025 15:09

myieye marked this pull request as draft November 5, 2025 16:29

myieye added 4 commits November 11, 2025 12:20

Only use 'main' Parts of speech. Like FLEx.

5f9f86a

This excludes e.g. Reversel index parts of speech.

Fix tests that create duplicate parts-of-speech

ab502bc

fixup! Only use 'main' Parts of speech. Like FLEx. This excludes e.g.…

48b8cf3

… Reversel index parts of speech.

Add test that reversal index Parts of speech are excluded

c552964

myieye force-pushed the only-use-main-parts-of-speech branch from 5f83837 to c552964 Compare November 11, 2025 11:20

myieye marked this pull request as ready for review November 11, 2025 11:42

myieye requested a review from Copilot November 11, 2025 11:43

Copilot started reviewing on behalf of myieye November 11, 2025 11:43 View session

Copilot finished reviewing on behalf of myieye November 11, 2025 11:45

Copilot AI reviewed Nov 11, 2025

View reviewed changes

backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs Show resolved Hide resolved

jasonleenaylor approved these changes Nov 18, 2025

View reviewed changes

myieye merged commit 589a34b into develop Nov 19, 2025
25 checks passed

myieye deleted the only-use-main-parts-of-speech branch November 19, 2025 11:55

Uh oh!

Only use 'main' Parts of speech. Like FLEx. #2084

Only use 'main' Parts of speech. Like FLEx. #2084

Conversation

myieye commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UI unit Tests

Uh oh!

argos-ci bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myieye commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myieye commented Nov 3, 2025

Uh oh!

coderabbitai bot commented Nov 3, 2025

Uh oh!

myieye commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

jasonleenaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myieye commented Oct 29, 2025 •

edited

Loading

coderabbitai bot commented Oct 29, 2025 •

edited

Loading

github-actions bot commented Oct 29, 2025 •

edited

Loading

argos-ci bot commented Oct 29, 2025 •

edited

Loading

myieye commented Nov 3, 2025 •

edited

Loading

myieye commented Nov 11, 2025 •

edited

Loading